Skip to content

added a new Eos constant option and ctest#370

Open
alicebarthel wants to merge 3 commits intoE3SM-Project:developfrom
alicebarthel:omega/add-constant-eos
Open

added a new Eos constant option and ctest#370
alicebarthel wants to merge 3 commits intoE3SM-Project:developfrom
alicebarthel:omega/add-constant-eos

Conversation

@alicebarthel
Copy link
Copy Markdown

@alicebarthel alicebarthel commented Mar 19, 2026

This PR adds a 3rd Eos option, 'constant', which simply return 1/RhoSw based on the GlobalConstants.h

Checklist

  • Documentation:
  • Linting
  • Building
    • CMake build works fine on pm-cpu
    • CMake error on pm-gpu
  • Testing
    • CTest unit tests: all tests passed on pm-cpu (gnu+Debug)
    • The Polaris omega_pr test suite: job 50277015 on pm-cpu_gnu --> 1 failed test
    • New tests:
      • Added a constant test in EOS_TEST

@alicebarthel
Copy link
Copy Markdown
Author

@cbegeman Is this what you had in mind? The ctest ran on pm-cpu (gnu) but I still need to do the other tests and the doc update. Can you have a look and see if that meets your needs?

@cbegeman
Copy link
Copy Markdown

That looks great! Thank you for getting to this so quickly! One more thing: could we put computeSpecVol someplace during runtime (or perhaps state initialization)? I think this is necessary before we have SpecVol included in the output, but let me know if I'm mistaken.

@alicebarthel
Copy link
Copy Markdown
Author

Do you mean adding the Eos in the OceanInit, e.g. at here alongside the StateInit? Or OceanRun?
I am not familiar with the driver structure so am unsure what the strategy is.
Yes, given that SpecVol is a data member of Eos rather than a state variable, we need Eos initialized before we store or output SpecVol. I am super familiar with IO machinery, looking into it right now.
Point me to the relevant code if you have somewhere where we already output other fields?

@cbegeman
Copy link
Copy Markdown

I think as long as they are initialized, the fields are registered and can be output. So I think it's sufficient to do it alongside StateInit. Maybe @brian-oneill can confirm?

@alicebarthel alicebarthel force-pushed the omega/add-constant-eos branch 2 times, most recently from 2b97a70 to c9daf77 Compare March 19, 2026 23:43
@alicebarthel
Copy link
Copy Markdown
Author

alicebarthel commented Mar 19, 2026

The latest doc will be at: https://portal.nersc.gov/project/e3sm/abarthel/doc/pr-add-constant-eos/html/
All Ctests passed on: pm-cpu/gnu (Debug).
Hitting issue #364 on pm-gpu

@hyungyukang
Copy link
Copy Markdown

@alicebarthel and @cbegeman , this is what I need for testing the implicit vertical mixing to match results with MPAS-Ocean. I was actually about to discuss this with @cbegeman . Thank you both for your work on this.

I’ll test this with time stepping after merging the branch into my testing branch and report back here.

@alicebarthel
Copy link
Copy Markdown
Author

alicebarthel commented Mar 20, 2026

the omega_pr suite on pm-cpu has 1 failed test: the baseline comparisons of ocean/planar/manufactured_solution/convergence_both/del4

@hyungyukang
Copy link
Copy Markdown

I tested this PR by cherry-picking it into my working branch (https://github.com/hyungyukang/Omega/tree/hkang/merge-eos-const-PR).

With EosType: constant, RhoSw * SpecVol becomes 1.0 during time stepping, so the implicit vertical mixing term is consistent with that in MPAS-Ocean.

@alicebarthel
Copy link
Copy Markdown
Author

Polaris omega_pr suite

  • Baseline workdir: /global/homes/a/abarthel/pscratch/polaris-scratch/baseline_omega_pr
  • Baseline build: /global/homes/a/abarthel/pscratch/polaris-scratch/baseline_omega_pr/build
  • PR build: /global/homes/a/abarthel/pscratch/omega_scratch/add-constant-eos-cpu-v2
  • PR workdir: /pscratch/sd/a/abarthel/polaris-scratch/pr-constant-eos-cpu-v2
  • Machine: pm-cpu
  • Compiler: gnu
  • Build type: <Debug|Release>
  • Log: /pscratch/sd/a/abarthel/polaris-scratch/pr-constant-eos-cpu-v2/polaris_omega_pr.o50838814
  • Result:
    • Diffs (1 of 8):
      • ocean/planar/manufactured_solution/convergence_both/del4

@alicebarthel
Copy link
Copy Markdown
Author

@cbegeman per your recommendation, I ran a polaris test adding- Eos in the forward.yaml.
I ran barotropic_gyre/munk/no-slip/short_forward, which ran successfully.
The output.nc file now includes the variables SpecVol and SpecVolDisplaced.
At this stage, these 2 variables are all zero values, which I think makes sense because we initialize these arrays at zero and have not included it in the timestepping in the present codebase.

I think that means that we have successfully tested the Eos init and output. @cbegeman you agree?

@alicebarthel
Copy link
Copy Markdown
Author

Do we want to merge this PR or should we merge it with @hyungyukang's branch/updates to timestepping?

@cbegeman
Copy link
Copy Markdown

cbegeman commented Apr 2, 2026

This PR adds a 3rd Eos option, 'constant', which simply return 1/RhoSw based on the GlobalConstants.h

Does this description suggest that we want to initialize to 1/RhoSw before merging? I think this would be desirable.

@alicebarthel
Copy link
Copy Markdown
Author

@cbegeman How does this look?
I could also add the default value in the computeSpecVol to be 1/RhoSw rather than zero. That was my first inclination but hopefully is not necessary and a good(?) way to catch errors down the line? Happy to add that modif too if that would be cleaner.
Here, I set it up in the init so that we can check the values before running computeSpecVol.
About to rebase. Waiting for the polaris test to check the array values.

@alicebarthel alicebarthel force-pushed the omega/add-constant-eos branch from a5372df to aebcd62 Compare April 7, 2026 00:28
@cbegeman
Copy link
Copy Markdown

cbegeman commented Apr 7, 2026

Great! My understand is that with these changes, SpecVol is initialized to 1/RhoSw but the CTest checks that if it is initialized to 0 that computeSpecVol results in a value of 1/RhoSw. I think that's a good way to catch issues. Let me know when you are ready for me to test.

@alicebarthel
Copy link
Copy Markdown
Author

Thanks Carolyn! The polaris test looks good and shows the initialization happens appropriately. I will do the first series of tests after the rebase and push asap.

@alicebarthel
Copy link
Copy Markdown
Author

Ctests pass on pm-cpu (gnu, Debug) and pm-gpu (Debug).
Omega_pr tests in the queue for pm-cpu_gnu.
@cbegeman would you be willing to run tests on your side, perhaps touching on other conbinations like ctests on pm-cpu_intel, omega_pr on gpu, or any other machines you have access to?

@alicebarthel alicebarthel marked this pull request as ready for review April 7, 2026 15:09
@alicebarthel alicebarthel requested a review from cbegeman April 7, 2026 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants